Repository navigation
Remove the unused restoring of the PPX context - #8859
Merged
Merged
Conversation
cknitt
force-pushed
the
remove-ppx-context-restore
branch
from
October 13, 2026 14:56
c23eb5e to
debb3cc
Compare
cknitt
marked this pull request as ready for review
October 13, 2026 14:56
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #8859 +/- ##
==========================================
+ Coverage 80.49% 80.51% +0.02%
==========================================
Files 447 447
Lines 61027 61060 +33
==========================================
+ Hits 49123 49162 +39
+ Misses 11904 11898 -6
🚀 New features to boost your workflow:
|
Member
Author
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Codex Review: Didn't find any major issues. 🎉 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
rescript
@rescript/belt
@rescript/darwin-arm64
@rescript/darwin-x64
@rescript/linux-arm64
@rescript/linux-x64
@rescript/runtime
@rescript/win32-x64
commit: |
|
Developer playground preview: https://rescript-lang.github.io/rescript/dev-playground/?version=pr-8859 |
fhammerschmidt
approved these changes
Oct 14, 2026
Every caller drops the context without restoring it, so the cookie store was never filled. PPXs still receive an empty cookies field. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WZbCuRQ1Tn31bnKzLj5e72 Signed-off-by: Christoph Knittel <ck@cca.io>
cknitt
force-pushed
the
remove-ppx-context-restore
branch
from
October 14, 2026 08:44
debb3cc to
25266eb
Compare
cknitt
enabled auto-merge (squash)
October 14, 2026 08:47
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The compiler sends every external PPX an
[@@@ocaml.ppx.context]attribute and drops it again from the PPX's output.Ast_mapper.drop_ppx_context_str/_sigcould also restore the context's data in the compiler (include dirs, load path, open modules,debugand cookies), but all four callers pass~restore:false. SoPpx_context.restorenever ran and the cookie store stayed empty: PPXs always gotcookies = [], and cookies they set were discarded.This removes the dead code:
Ppx_context.restore, the cookie store and the~restoreparameter ofCmd_ppx_apply.apply_rewritersandAst_mapper.drop_ppx_context_*. The context PPXs receive is unchanged; it still carries an emptycookiesfield, and its strings still go through the v0 mapper as plain semantic strings.Found while making compiler state per-request for in-process compilation (#8781), where the cookie store was one of the globals to isolate.
Tests
tests/ounit_tests/ounit_ast_mapper0_tests.mlpins the v0 context a PPX receives: its field names, and an emptycookieslist.🤖 Generated with Claude Code